Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

src: refactor macro to std::min in node_buffer.cc #25919

Closed
wants to merge 1 commit into from
Closed

src: refactor macro to std::min in node_buffer.cc #25919

wants to merge 1 commit into from

Conversation

gengjiawen
Copy link
Member

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Feb 4, 2019
@addaleax
Copy link
Member

addaleax commented Feb 4, 2019

We use std::min elsewhere in our code, maybe it’s best to just replace the macro usage with that?

@gengjiawen
Copy link
Member Author

@addaleax I will give it a try.

@gengjiawen gengjiawen force-pushed the fix_macro_define branch 2 times, most recently from 04407e9 to 57b2d35 Compare February 4, 2019 10:35
@gengjiawen gengjiawen changed the title src: avoid macro re-define in node_buffer.cc src: refactor macro to std::min in node_buffer.cc Feb 4, 2019
bnoordhuis
bnoordhuis approved these changes Feb 4, 2019
View reviewed changes
src/node_buffer.cc Outdated Show resolved Hide resolved
@refack
Copy link
Contributor

refack commented Feb 4, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/20573/

ChALkeR
ChALkeR reviewed Feb 7, 2019
View reviewed changes
src/node_buffer.cc Outdated Show resolved Hide resolved
@gengjiawen
Copy link
Member Author

Since we can't now format the whole file, I run into some issue on windows wsl:

Formatting C++ diff from HEAD..
  File "tools/clang-format/node_modules/.bin/git-clang-format", line 2
    basedir=$(dirname "$(echo "$0" | sed -e 's,\\,/,g')")
            ^
SyntaxError: invalid syntax
Makefile:1254: recipe for target 'format-cpp' failed
make: *** [format-cpp] Error 1

@addaleax
Copy link
Member

addaleax commented Feb 9, 2019

@gengjiawen That is npm trying to work around the lack of symlinks on Windows…

If you look at the definition of format-cpp in the Makefile, I think you might be able to resolve this by replacing .bin/git-clang-format and .bin/clang-format with their respective targets (clang-format/bin/git-clang-format and clang-format/index.js)?

@gengjiawen
Copy link
Member Author

gengjiawen commented Feb 9, 2019 •
edited
Loading

@addaleax Thanks. I already clang-format the whole file. revert unrelated change. Easier when I get help from CLion :)

@gengjiawen
Copy link
Member Author

In the long run we should will try to make it more easier as described in #25908. I will try to invest more on this recently.

@addaleax
Copy link
Member

addaleax commented Feb 9, 2019

Landed in 2fa6376

@addaleax addaleax closed this Feb 9, 2019
addaleax pushed a commit that referenced this pull request Feb 9, 2019
@gengjiawen @addaleax
src: refactor macro to std::min in node_buffer.cc
2fa6376 
PR-URL: #25919
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
targos pushed a commit that referenced this pull request Feb 10, 2019
@gengjiawen @targos
src: refactor macro to std::min in node_buffer.cc
779a577 
PR-URL: #25919
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
@targos targos mentioned this pull request Feb 14, 2019
Merged
@beevelop beevelop mentioned this pull request Feb 15, 2019
Closed
This was referenced Feb 15, 2019
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

@ChALkeR ChALkeR ChALkeR approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@gengjiawen @addaleax @refack @bnoordhuis @ChALkeR @jasnell @lpinca @cjihrig @nodejs-github-bot